refactor: use requires to assert and automatically log richer information on failure#65
refactor: use requires to assert and automatically log richer information on failure#65
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (44)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dodecahedr0x
left a comment
There was a problem hiding this comment.
require_n_accounts is more restrictive than the current implem. I don't thinks it's a bad thing (tx data is already a limit) but it's also unnecessary IMO in this PR
Plus small suggestions
| #[macro_export] | ||
| macro_rules! require { | ||
| ($cond:expr, $error:expr) => {{ | ||
| if !$cond { |
There was a problem hiding this comment.
they can all use core::hiint::unlikely to improve perfs (tried it locally, up to 600 CU gains on some instructions, requires nightly)
There was a problem hiding this comment.
Yes. I could use that. But 600 CU is very unlikely (pun not intended), because if usually consumes 4 to 6 CU.
There was a problem hiding this comment.
I'm not addressing this in this PR. Will consider doing it in the coming PRs. We'll probably move requires.rs to some common library crate.
There was a problem hiding this comment.
Sure, can be done in another PRs, it just felt appropriate since introducing all the requires.
FYI, here's the benchmark by just adding unlikely. It probably indicates that either the compiler does some weird stuff, or we are doing too many checks on some paths.
───────────────────────────────────────────────────────────────────────────────
Key Baseline CU Current CU Δ CU Δ %
───────────────────────────────────────────────────────────────────────────────
close_eata::close 1,726 1,723 -3 -0.17%
create_eata_perm::default 13,709 13,569 -140 -1.02%
create_eata_perm::permissionless 24,198 24,058 -140 -0.58%
del_eata::delegate 43,435 43,337 -98 -0.23%
del_eata::non_owner 29,935 29,837 -98 -0.33%
del_eata::redelegate 152 152 +0 +0.00%
del_eata::reinit 7,713 7,716 +3 +0.04%
del_eata_perm::delegate 48,487 48,485 -2 -0.00%
del_eata_perm::non_owner 52,987 52,985 -2 -0.00%
del_eata_perm::redelegate 175 174 -1 -0.57%
del_shuttle_eata::delegate 25,546 25,434 -112 -0.44%
del_shuttle_eata::redelegate 173 160 -13 -7.51%
del_shuttle_merge::delegate 80,548 79,861 -687 -0.85%
del_shuttle_priv::queue 27,991 27,890 -101 -0.36%
del_shuttle_priv::shuttle 128,773 128,086 -687 -0.53%
del_tq::delegate 23,491 23,390 -101 -0.43%
del_tq::redelegate 1,793 1,794 +1 +0.06%
dep_queue::accepts_legacy_destination_ata 13,509 13,503 -6 -0.04%
dep_queue::all_splits_use_client_ref_id 10,980 10,976 -4 -0.04%
dep_queue::assign_group_id_0 10,712 10,706 -6 -0.06%
dep_queue::assign_group_id_1 11,180 11,174 -6 -0.05%
dep_queue::deterministic_delays 11,383 11,377 -6 -0.05%
dep_queue::once_split 10,953 10,947 -6 -0.05%
dep_queue::reject_delay_range 576 574 -2 -0.35%
dep_queue::reject_queue_full 289 282 -7 -2.42%
dep_queue::reject_split_gt_amt 573 572 -1 -0.17%
dep_queue::reject_zero_split 571 569 -2 -0.35%
dep_queue::return_to_shuttle 7,851 7,844 -7 -0.09%
dep_queue::split3_mod5 11,152 11,146 -6 -0.05%
dep_queue::split4_mod5 11,504 11,498 -6 -0.05%
dep_shuttle::deposit 7,861 7,858 -3 -0.04%
dep_spl::deposit 7,861 7,858 -3 -0.04%
init_eata::init 4,723 4,588 -135 -2.86%
init_gvault::init 15,931 15,795 -136 -0.85%
init_gvault::migrate 12,998 12,863 -135 -1.04%
init_shuttle::init 19,009 18,733 -276 -1.45%
init_tq::custom 14,997 14,991 -6 -0.04%
init_tq::default 13,530 13,524 -6 -0.04%
init_tq::noop 6,424 6,420 -4 -0.06%
lamports_pda::allow_extra_lamports 1,876 1,834 -42 -2.24%
lamports_pda::sponsored_lamports_transfer 61,282 60,894 -388 -0.63%
lamports_pda::transfer_lamports_pda 6,376 6,334 -42 -0.66%
merge_shuttle::merge 9,489 9,456 -33 -0.35%
rent_pda::init 1,631 1,632 +1 +0.06%
rent_pda::reinit 240 241 +1 +0.42%
reset_eata_perm::reset 26,531 26,389 -142 -0.54%
tq_alloc::already_allocated 1,837 1,837 +0 +0.00%
tq_alloc::first_allocate 19,030 19,030 +0 +0.00%
tq_auto::crank_1 5,331 5,332 +1 +0.02%
tq_auto::crank_2 6,497 6,498 +1 +0.02%
tq_auto::crank_bad_magic 807 807 +0 +0.00%
tq_auto::enqueue_delayed 10,437 10,431 -6 -0.06%
tq_auto::enqueue_ready 10,437 10,431 -6 -0.06%
tq_auto::enqueue_with_client_ref_id 10,438 10,434 -4 -0.04%
tq_auto::run_scheduled_tick 15,323 15,319 -4 -0.03%
tq_auto::run_scheduled_tick_via_magic_bundle 15,321 15,317 -4 -0.03%
tq_auto::schedule 5,331 5,332 +1 +0.02%
tq_auto::schedule_with_client_ref_id 5,331 5,332 +1 +0.02%
tq_auto::tick_after_bundle 2,112 2,106 -6 -0.28%
tq_auto::tick_empty 2,112 2,106 -6 -0.28%
tq_auto::tick_not_ready 2,198 2,192 -6 -0.27%
ud_wd_close::undelegate 13,765 13,743 -22 -0.16%
undel_eata_cb::undelegate 36,856 36,856 +0 +0.00%
undel_perm_cb::undelegate 34,444 34,444 +0 +0.00%
wd_shuttle::withdraw 64,188 63,703 -485 -0.76%
wd_spl::withdraw 7,885 7,874 -11 -0.14%
───────────────────────────────────────────────────────────────────────────────There was a problem hiding this comment.
Wow.. that is interesting! sBPF keeps surprising me!

Question:
Are we passing more accounts than needed by processors?Based on the existing code, I had to use.require_n_accounts_with_ignored!()but I want to use the stricter formrequire_n_accounts!()